respect attribute namespace during migration#5066
Conversation
SimonCropp
had a problem deploying
to
Pull Requests
GitHub Actions
Failure
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review: respect attribute namespace during migration
Summary: This PR fixes a real and non-obvious bug — the migration analyzers were matching attributes by name only, so a user-defined [MyCompany.Attributes.Ignore] would get incorrectly migrated as if it were [NUnit.Framework.Ignore]. Using the semantic model to verify the containing namespace before converting is the right approach.
Design
The chosen architecture is clean:
- A single abstract
IsFrameworkNamespace(string? ns)to keep framework-specific knowledge in each concrete analyzer - A concrete
IsFrameworkAttribute(AttributeSyntax node)in the base class handles the semantic lookup and composes the null-fallback logic centrally - The guard is placed before both the removal check and the conversion check, so the fix is applied consistently to both paths
This is exactly the right level of abstraction. No concerns here.
One Minor Observation
IsFrameworkNamespace has a string? parameter, but the base-class caller already guards the null case:
var ns = symbol?.ContainingType?.ContainingNamespace?.ToDisplayString();
if (ns == null)
return true; // If we can't resolve, assume framework (preserves existing behavior)
return IsFrameworkNamespace(ns); // ns is guaranteed non-null hereAll three implementations then re-check ns != null defensively, which is harmless but redundant. The parameter could be string (non-nullable) since the abstract method is never called with null. This is a very minor point and either style is acceptable — nullable is arguably safer as a defensive API surface.
Fallback Behavior
Returning true (treat as framework attribute) when the symbol can't be resolved is the right conservative default. It preserves existing behavior for edge cases where semantic information isn't available, avoiding false negatives in the migration tool.
Test Coverage
The parameterized tests are well-chosen — they enumerate the most common attribute names that could realistically collide with user-defined types, and verify the full round-trip through the code fixer. Coverage spans all three frameworks. Good work.
Verdict: This is a correct, well-structured fix with solid tests. The only observation above is cosmetic and doesn't warrant a change. Approving.
There was a problem hiding this comment.
Code Review: respect attribute namespace during migration
Summary: This PR fixes a real and non-obvious bug — the migration analyzers were matching attributes by name only, so a user-defined [MyCompany.Attributes.Ignore] would get incorrectly migrated as if it were [NUnit.Framework.Ignore]. Using the semantic model to verify the containing namespace before converting is the right approach.
Design
The chosen architecture is clean:
- A single abstract
IsFrameworkNamespace(string? ns)keeps framework-specific knowledge in each concrete analyzer - A concrete
IsFrameworkAttribute(AttributeSyntax node)in the base class handles the semantic lookup and composes the null-fallback logic centrally - The guard is placed before both the removal check and the conversion check, so the fix is applied consistently to both paths
This is exactly the right level of abstraction. No concerns here.
One Minor Observation
IsFrameworkNamespace has a string? parameter, but the base-class caller already guards the null case:
var ns = symbol?.ContainingType?.ContainingNamespace?.ToDisplayString();
if (ns == null)
return true; // If we can't resolve, assume framework (preserves existing behavior)
return IsFrameworkNamespace(ns); // ns is guaranteed non-null hereAll three implementations then re-check ns != null defensively, which is harmless but redundant. The parameter could be string (non-nullable) since the abstract method is never called with null. Minor point — either style is acceptable.
Fallback Behavior
Returning true (treat as framework attribute) when the symbol can't be resolved is the right conservative default. It preserves existing behavior for edge cases where semantic information isn't available, avoiding false negatives in the migration tool.
Test Coverage
The parameterized tests are well-chosen — they enumerate the most common attribute names that could realistically collide with user-defined types, and verify the full round-trip through the code fixer. Coverage spans all three frameworks (NUnit, MSTest, xUnit). Good work.
Verdict: Correct, well-structured fix with solid tests. Approved.
|
Thanks! |
SimonCropp
deleted the
NUnit_Ignore_From_Different_Namespace_Not_Converted
branch
Updated [TUnit](https://github.com/thomhurst/TUnit) from 1.18.9 to 1.18.21. <details> <summary>Release notes</summary> _Sourced from [TUnit's releases](https://github.com/thomhurst/TUnit/releases)._ ## 1.18.21 <!-- Release notes generated using configuration in .github/release.yml at v1.18.21 --> ## What's Changed ### Other Changes * avoid some string alloc by using AppendJoin by @SimonCropp in thomhurst/TUnit#4971 * respect attribute namespace during migration by @SimonCropp in thomhurst/TUnit#5066 * fix: parent data source initialization spans under session span by @thomhurst in thomhurst/TUnit#5071 ### Dependencies * chore(deps): update tunit to 1.18.9 by @thomhurst in thomhurst/TUnit#5060 * chore(deps): update dependency polyfill to 9.14.0 by @thomhurst in thomhurst/TUnit#5063 * chore(deps): update dependency microsoft.testing.extensions.codecoverage to 18.5.2 by @thomhurst in thomhurst/TUnit#5065 * chore(deps): update dependency polyfill to 9.14.0 by @thomhurst in thomhurst/TUnit#5062 * chore(deps): update actions/github-script action to v8 by @thomhurst in thomhurst/TUnit#5053 * chore(deps): update dependency polyfill to 9.15.0 by @thomhurst in thomhurst/TUnit#5067 * chore(deps): update dependency polyfill to 9.15.0 by @thomhurst in thomhurst/TUnit#5068 **Full Changelog**: thomhurst/TUnit@v1.18.9...v1.18.21 Commits viewable in [compare view](thomhurst/TUnit@v1.18.9...v1.18.21). </details> [](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) </details> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
2 participants
No description provided.